-
-
Notifications
You must be signed in to change notification settings - Fork 29k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add config flow to synology_srm #35412
Conversation
b03eef2
to
50d8a7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config_flow.py
needs 100% coverage : https://codecov.io/gh/home-assistant/core/compare/9eb1505aa13d8d3f60ab31ca92d9598fc03852e1...50d8a7f19836b4e70ecd47491db27942c8fb5cb9/diff
Did not look at tests.
Nice work 🎉
Thanks @Quentame for your feedback. I've updated the PR will all your remarks and I've added more tests to increase coverage up to 100%! |
afbd2cd
to
c101b5f
Compare
@@ -71,3 +76,9 @@ def get_srm_client_from_user_data(data) -> synology_srm.Client: | |||
client.http.disable_https_verify() | |||
|
|||
return client | |||
|
|||
|
|||
def fetch_srm_device_id(client: SynologyClient): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you putting this function here since it's used only in the confug_flow ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the function because it's always mocked in tests to I cannot have 100% coverage in this situation. It's also make sense in the end because I've another function just at the top to generate the client, so both functions handling the SynologyClient
are at the same place.
@@ -46,21 +35,18 @@ async def async_step_user(self, user_input=None): | |||
try: | |||
client = get_srm_client_from_user_data(user_input) | |||
device_id = await self.hass.async_add_executor_job( | |||
_login_and_fetch_device_id, client | |||
fetch_srm_device_id, client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the device_id
actually ?
A MAC adress, serial number, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
device_id
can be compared with a serial number. For instance, it's in the following format synology_irr436p_rt2600ac
. It's much easier to fetch it from the API with a simple endpoint and the device ID is also written in the back of the device so it's a good device ID to display to the user IMO.
c101b5f
to
9377088
Compare
I pushed the change and rebased into one commit. |
Please don't squash commits when someone starts to review your PR, so we can follow the diff better (GitHub does not have the diff tool as GitLab has). I agree this can be kinda ugly sometimes but no worries your commits will be squashed at merge 😉 |
So then, you should not force push except if you rebase (not interactively) |
Right, my bad! Forgot about that. I only pushed the change on https://github.com/home-assistant/core/pull/35412/files#diff-1262a40b266ddf89f36e596d9c1b0d62R46. |
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Still waiting for comments 😄 |
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
I'm not using the Synology SRM router anymore and it seems nobody wants this PR merged so I will close it. |
Sorry @aerialls I was very busy those weeks 😢 @i00 Would you like to create a new PR for this ? Ping me when it's done (also available on Discord, found me in the official HA server https://discord.gg/KxdbHtS) |
Breaking change
The Synology SRM integration is now "config flow" compatible. The previous YAML integration is done and the configuration should be done in the UI and not in the YAML file.
Proposed change
Type of change
Migration to the "config flow" workflow. Support addition, updates and removal of entities (even if I would like feedbacks on the latest because I'm not sure of the global workflow).
Example entry for
configuration.yaml
:# Example configuration.yaml
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
PR incoming to update the documentation.
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale: